Skip to content

Add early spatial_extent validation for load_collection/validation with lenient default EPSG:4326 bounds#501

Merged
jdries merged 7 commits into
masterfrom
copilot/check-common-errors-spatial-extent
May 19, 2026
Merged

Add early spatial_extent validation for load_collection/validation with lenient default EPSG:4326 bounds#501
jdries merged 7 commits into
masterfrom
copilot/check-common-errors-spatial-extent

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 14, 2026

  • Review new PR comment and current branch state.
  • Check recent GitHub Actions workflow run status/logs for CI context.
  • Add a changelog entry for the spatial extent validation update.
  • Run focused validation (targeted test/lint as applicable).
  • Run security validation and push changes.
  • Reply to the new PR comment with commit hash.

Copilot AI linked an issue May 14, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Check for common errors in spatial extent Add early spatial_extent validation for common user input errors in load_collection//validation May 14, 2026
Copilot finished work on behalf of jdries May 14, 2026 18:05
Copilot AI requested a review from jdries May 14, 2026 18:05
@jdries jdries marked this pull request as ready for review May 14, 2026 18:12
@jdries
Copy link
Copy Markdown
Contributor

jdries commented May 19, 2026

@copilot one of the unit tests fails with:

  assert expected_message_part in errors[0]["message"]

E assert 'Expected dictionary/mapping but got list.' in "The value passed for parameter 'spatial_extent' in process 'load_collection' is invalid: Expected dictionary/mapping but got <class 'list'>."

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 19, 2026

@copilot one of the unit tests fails with:

  assert expected_message_part in errors[0]["message"]

E assert 'Expected dictionary/mapping but got list.' in "The value passed for parameter ...

Fixed in c1fda75. I relaxed that assertion to check for the stable prefix ("Expected dictionary/mapping but got") so it no longer fails on minor formatting/truncation differences in the type suffix.

Copilot finished work on behalf of jdries May 19, 2026 06:29
Copilot AI requested a review from jdries May 19, 2026 06:29
@jdries
Copy link
Copy Markdown
Contributor

jdries commented May 19, 2026

@copilot on this check:
d["west"] < -180 or d["west"] > 180 or d["east"] < -180 or d["east"] > 180
or d["south"] < -90 or d["south"] > 90 or d["north"] < -90 or d["north"] > 90

be more lenient on the valid range, to avoid breaking jobs. Allow extra 10 degrees latitude and +-180 degrees longitude

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 19, 2026

@copilot on this check:
d["west"] < -180 or d["west"] > 180 or d["east"] < -180 or d["east"] > 180
or d["south"] < -90 or d["south"] > 90 or d["north"] < -90 or d["north"] > 90
...

Implemented in 8f28040. I made the default (no crs) sanity bounds more lenient to allow longitude in [-360, 360] and latitude in [-100, 100], and added validation tests to ensure these ranges no longer raise ProcessParameterInvalid.

Copilot AI changed the title Add early spatial_extent validation for common user input errors in load_collection//validation Add early spatial_extent validation for load_collection/validation with lenient default EPSG:4326 bounds May 19, 2026
Copilot finished work on behalf of jdries May 19, 2026 06:42
@jdries
Copy link
Copy Markdown
Contributor

jdries commented May 19, 2026

@copilot add a changelog entry

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 19, 2026

@copilot add a changelog entry

Done in 37fd059: I added a changelog entry under ## In progress: 0.140.0 for the earlier spatial_extent validation and lenient default EPSG:4326 bounds update.

Copilot finished work on behalf of jdries May 19, 2026 07:14
Copilot AI requested a review from jdries May 19, 2026 07:14
@jdries jdries merged commit be8fc66 into master May 19, 2026
@jdries jdries deleted the copilot/check-common-errors-spatial-extent branch May 19, 2026 07:19
Comment on lines +93 to +97
if d["west"] >= d["east"]:
raise ProcessParameterInvalidException(
parameter=field,
process=process_id,
reason=f"'west' must be smaller than 'east', but got west={d['west']!r} and east={d['east']!r}.",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to break anti-meridian use cases

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall I delete the check, or do you have an example of such a use case?

soxofaan added a commit to Open-EO/openeo-geopyspark-driver that referenced this pull request May 19, 2026
@EmileSonneveld
Copy link
Copy Markdown
Member

EmileSonneveld commented May 19, 2026

This also broke an integration test where north and south are the same value:

"The value passed for parameter 'spatial_extent' in process "
  +         "'load_collection' is invalid: 'south' must be smaller than 'north', "
  +         'but got south=-45.2 and north=-45.2.',

@soxofaan Should this be allowed? EDIT: Removed graph from validation list.

@jdries
Copy link
Copy Markdown
Contributor

jdries commented May 19, 2026 via email

@EmileSonneveld
Copy link
Copy Markdown
Member

@jdries Mr to avoid errors when north and south are the same: #504

jdries pushed a commit that referenced this pull request May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check for common errors in spatial_extent

4 participants